-
Notifications
You must be signed in to change notification settings - Fork 2.3k
new plugin: safe_autoenv #2354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
new plugin: safe_autoenv #2354
Conversation
Exports variables from .env when starting a shell or cd'ing to a directory. When leaving the directory the variables from .env will be unset. Given: $ mkdir -pv /tmp/foo/bar/baz mkdir: created directory '/tmp/foo' mkdir: created directory '/tmp/foo/bar' mkdir: created directory '/tmp/foo/bar/baz' $ echo CLOWN=sideshowbob > /tmp/foo/bar/baz/.env $ echo MASTER_CLOWN="Krusty the Clown" > /tmp/foo/bar/.env $ cd /tmp/foo/bar/baz/ Processing /tmp/foo/bar/.env Processing /tmp/foo/bar/baz/.env $ cd ../ Unsetting CLOWN (from /tmp/foo/bar/baz) $ cd baz/ Processing /tmp/foo/bar/baz/.env cd ../../ Unsetting MASTER_CLOWN (from /tmp/foo/bar) Unsetting CLOWN (from /tmp/foo/bar/baz) Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
@oz123 ? |
**safe_autoenv.plugin.bash:** - SC2207: Added shellcheck disable for array assignment from command substitution - SC2155: Separated declaration and assignment for env_dir variable **artisan.completion.bash:** - SC2034: Removed unused disable comment for 'commands' variable - SC2016: Changed single quotes to double quotes for proper variable expansion All shellcheck warnings resolved. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed shellcheck warnings (SC2207, SC2155) and artisan completion issues (SC2034, SC2016). All pre-commit hooks now pass. ✅ |
Hi! Thanks for fixing the stuff for me. I already did some of it but have not pushed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just quickly checked. I haven't checked the logics.
declare _AUTOENV_LAST_PWD="" # Track last working directory | ||
|
||
_autoenv_init() { | ||
typeset target home _file current_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeset
is used here, while local
is used elsewhere. This should be normalized.
home="${HOME%/*}" | ||
|
||
# shellcheck disable=SC2207 | ||
_files=($( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work when the directory path contains spaces or glob characters. It is better to use while [[ ... ]]; do ... _files+=("${_file}") ... done
than to echo
the filename and split it using _files=($(...))
.
# Validate key name (additional safety check) | ||
if [[ ! "$key" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then | ||
echo "Warning: Invalid variable name '$key' at line $line_number in $env_file" >&2 | ||
continue | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what situation does this happen? I think key
is supposed to contain a valid key name in this context.
# Handle quoted values (remove outer quotes if present) | ||
if [[ "$value" =~ ^\"(.*)\"$ ]]; then | ||
value="${BASH_REMATCH[1]}" | ||
elif [[ "$value" =~ ^\'(.*)\'$ ]]; then | ||
value="${BASH_REMATCH[1]}" | ||
fi | ||
|
||
# Export the variable silently | ||
export "$key=$value" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails when value
contains escape sequences such as key="\""
. This fails when value
contains multiple '...'
pairs, such as key='abc'\''def'
. This can simply be
# Handle quoted values (remove outer quotes if present) | |
if [[ "$value" =~ ^\"(.*)\"$ ]]; then | |
value="${BASH_REMATCH[1]}" | |
elif [[ "$value" =~ ^\'(.*)\'$ ]]; then | |
value="${BASH_REMATCH[1]}" | |
fi | |
# Export the variable silently | |
export "$key=$value" | |
# Export the variable silently | |
eval "export $line" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Oz meant that it unsets vars once you CD out of a directory, however I don't see how it restores a previous value. needs a stack and all.
I now guess that the above part of the code corresponds to the safety.
A typical vulnerability of this type of configuration (such as direnv and autoenv) is that it can load a malicious environment just by cd'ing into the directory. For example, one may download a wild tarball from the internet, extract it, and cd into the directory, assuming that just cd'ing into the directory doesn't execute any code from the tarball. However, if the environment contains key=$(some malicious code)
, a naive implementation of eval
may execute the malicious code.
However, even if the code were not directly executed when the environment is loaded, there are many variables that may be interpreted as commands and executed by applications, such as LESSOPEN
, EDITOR
, VISUAL
, PAGER
, etc. Or, LD_PRELOAD
in Linux can be used to inject code on an arbitrary exec
call. Just setting an environment value in the form export key="$value"
isn't actually so safe.
To solve the issue, the framework should require the user to explicitly trust each directory environment (.env
) by running something like autoenv trust <dir>
and manage the list of sha256 codes of the trusted .env
in a safe directory. Or there might be another approach, but I don't have an idea now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand the use of repeating the export (or using eval "export ..."
if the code is changed to that) here. If the user chooses to write GITHUB_TOKEN=...
, I think it would be unexpected for that to be silently converted to an environment variable.
As @akinomyoga mentioned there are security concerns, and autoenv
already has a system for allowing or disabling the sourcing of files by their checksum. I think duplicating that here just so some variables are marked as "exported" would be a lot of duplicate work and potentially confusing for the end user.
;; | ||
"status") | ||
echo "Processed directories:" | ||
for dir in "${!_AUTOENV_PROCESSED_DIRS[@]}"; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dir
is not localized.
done | ||
echo | ||
echo "Variable sources:" | ||
for var in "${!_AUTOENV_VAR_SOURCES[@]}"; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var
is not localized.
I guess @hyperupcall (who is a contributor of Bash-it and also the maintainer of By the way, the plugin name contains |
Yes, I am just about to publish 3.2.0 and if it's fixed by Friday (see all the remarks from @akinomyoga ) it will be in there. have a go... |
I think Oz meant that it unsets vars once you CD out of a directory, however I don't see how it restores a previous value. needs a stack and all. |
# Reinitialize arrays | ||
declare -A _AUTOENV_PROCESSED_DIRS | ||
declare -A _AUTOENV_DIR_VARS | ||
declare -A _AUTOENV_VAR_SOURCES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do what is intended. This doesn't initialize global associative arrays. This creates local associative arrays, which are never referenced later. To achieve what is expected, one needs to specify the -g
flag, but it requires Bash 4.2+, which is an even stronger requirement than 3.2+.
Summary of findings: Runtime code requiring bash 3.3+:
All runtime plugins requiring bash 3.3+ now have appropriate version checks and early returns. |
Thank you for your investigations! For pack.plugin.bash: OK, I haven't noticed there is an existing check for the version check in I checked how the associative arrays |
Anyway, we need a Bash version check also for this new plugin (as long as |
I'll read it a bit later, I have a feeling shellcheck would have caught any problems like that. However, right now I am glued to the TV, as a long war is coming to an end here and it's a happy news day I can't easily rip myself out of and do work :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like to be a useful plugin, the main autoenv
project does not implement unset-ing of variables or cleaning up in general when leaving directories. But I think the name "safe_autoenv " is a bit misleading. Yes, this code unsets variables that are set, but it also silently exports variables as environment variables, which potentially exposes more processes to that possible secret data. Perhaps "autounset-autoenv" or "autocleanup-autoenv" fit better?
To the bash-it maintainers: Would it be possible to note somewhere that this optionally "extends" autoenv and maybe compare it with the built-in AUTOENV_ENV_LEAVE_FILENAME
? For example. our .env.leave
requires manual unsettinng of variables, and this automatic detection is more convenient - but it also may cause unintended behavior. For example, if GITHUB_TOKEN
is set in ~/.bashrc
, and if it is overriden by a .env
file, this code may unset it for the whole shell session unless ~/.bashrc
is sourced again.
# Handle quoted values (remove outer quotes if present) | ||
if [[ "$value" =~ ^\"(.*)\"$ ]]; then | ||
value="${BASH_REMATCH[1]}" | ||
elif [[ "$value" =~ ^\'(.*)\'$ ]]; then | ||
value="${BASH_REMATCH[1]}" | ||
fi | ||
|
||
# Export the variable silently | ||
export "$key=$value" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand the use of repeating the export (or using eval "export ..."
if the code is changed to that) here. If the user chooses to write GITHUB_TOKEN=...
, I think it would be unexpected for that to be silently converted to an environment variable.
As @akinomyoga mentioned there are security concerns, and autoenv
already has a system for allowing or disabling the sourcing of files by their checksum. I think duplicating that here just so some variables are marked as "exported" would be a lot of duplicate work and potentially confusing for the end user.
} | ||
|
||
# Public function for manual use | ||
autoenv() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep autoenv
reserved in case the main autoenv project chooses to use it in the future. Perhaps safe-autoenv
or safeenv
are good alternatives?
# Unset each variable from this directory | ||
for var in $var_list; do | ||
if [[ "${_AUTOENV_VAR_SOURCES[$var]}" == "$dir" ]]; then | ||
echo "Unsetting $var (from $dir)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better UX if print statements were prefixed with "safe_autoenv" so it is clear who is printing to standard output when cd
ing to different directoreis (people may also confuse it with autoenv
if it is not prefixed)
} | ||
|
||
# Clean up variables from directories we've left | ||
_autoenv_cleanup() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if the function were prefixed with something like _safe_autoenv_
so it's clear that this is separate from the main autoenv project
LIFO direnv... |
Exports variables from .env when starting a shell or cd'ing to a directory.
When leaving the directory the variables from .env will be unset.
Given:
$ mkdir -pv /tmp/foo/bar/baz
mkdir: created directory '/tmp/foo'
mkdir: created directory '/tmp/foo/bar'
mkdir: created directory '/tmp/foo/bar/baz'
$ echo CLOWN=sideshowbob > /tmp/foo/bar/baz/.env
$ echo MASTER_CLOWN="Krusty the Clown" > /tmp/foo/bar/.env
$ cd /tmp/foo/bar/baz/
Processing /tmp/foo/bar/.env
Processing /tmp/foo/bar/baz/.env
$ cd ../
Unsetting CLOWN (from /tmp/foo/bar/baz)
$ cd baz/
Processing /tmp/foo/bar/baz/.env
cd ../../
Unsetting MASTER_CLOWN (from /tmp/foo/bar)
Unsetting CLOWN (from /tmp/foo/bar/baz)
Description
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
clean_files.txt
and formatted it usinglint_clean_files.sh
.